-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce MismatchingDataValueTypeException #7
Conversation
to be used by ValueFormatters
This might be generic enough to also be used for parsers. If so, I am not quite sure where we should put this? |
Where do you intend to throw this? In the format method of ValueFormatter implementations? If so, then it should derive from the base formatting exception. I just noticed a pile of fail with regard to that exception though. It is not referred to in the interface and is in the wrong namespace. That definitely needs to be fixed |
we plan to use this for ValueFormatters. I can have it derive from the base formatting exception and maybe fix the fail also :) |
* @param string $message | ||
* @param \Exception $previous | ||
*/ | ||
public function __construct( $expectedValueType, $dataValueType, $message = '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please construct a meaningful $message from $expectedValueType and $dataValueType if not given (perhaps don't even allow the $mesage to be given explicitly, or just include the explicit one in the auto-generated one).
renamed the exception and made the message more informative. I had this extend InvalidArgumentException (somewhat agree) in an earlier patch but it's also desired to extend FormattingException (which is a RuntimeException). Being php, suppose we can't do everything here. |
$this->expectedValueType = $expectedValueType; | ||
$this->dataValueType = $dataValueType; | ||
|
||
$message = "DataValueType $dataValueType does not match expected value " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like DataValueType is a class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I'd just drop the DataValueType type part, not like repeating that is helping clarity anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MismatchingException? MismatchingDataValueException? BadDataValueTypeException as daniel suggests? or what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you are misplacing some of your comments...
@thiemowmde what should we call it instead of DataValueType? |
* @licence GNU GPL v2+ | ||
* @author Katie Filbert < aude.wiki@gmail.com > | ||
*/ | ||
class DataValueTypeMismatchException extends FormattingException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MismatchingDataValueTypeException? The current one is in the format of ArgumentInvalidException, which is odd
Introduce MismatchingDataValueTypeException
to be used by ValueFormatters